chore: post-review cleanup + subtitle fail-safe and import reliability - #4
Conversation
…toggle - Remove effector/effector-react (never imported anywhere) - Gitignore and untrack packages/web/out generated files (flagged in Lumen's review for trailing whitespace — generated output shouldn't be committed) - Wire up the Show progress bar toggle that was a no-op callback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… on startup Two reliability fixes ahead of store submission: 1. Native subtitle hiding now requires an `inkahsubs-active` class that is only set while Inkah is actively rendering cues. If the subtitle pipeline fails (e.g. a Netflix player change breaks interception), users keep Netflix's native subtitles instead of getting nothing. 2. Dictionary imports now resume on every service worker startup, not only onInstalled. MV3 kills workers mid-import (Vicon alone is 61MB / 707K entries) and nothing ever retried — leaving Korean lookups silently empty. Per-table progress flags resume where the interrupted run stopped; an in-flight guard dedupes the onInstalled/startup race. Adds regression tests for import resume, early-return, and dedupe. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
conoremclaughlin
changed the title
conoremclaughlin
left a comment
conoremclaughlin
left a comment
There was a problem hiding this comment.
Thanks for the update. The cleanup and subtitle no-cues fail-safe direction look good, and local validation is green after WXT preparation / env normalization. I found one P1 in the new import-resume path around stale negative dictionary caches; details inline.
- YouTube MAIN world injector had no inkahsubsSeek listener, so the right panel play carets and progress bar cue clicks did nothing on YouTube (only Netflix handled the event). Seek via movie_player.seekTo with a raw video.currentTime fallback. - Right panel now re-mounts when toggled: YouTube's SPA rebuilds the sidebar DOM which detached a previously mounted panel, and toggling before cues loaded meant no panel existed to render into. Previously both cases required a full page refresh. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
conoremclaughlin
left a comment
conoremclaughlin
left a comment
There was a problem hiding this comment.
Re-reviewed f5e9959.
No additional findings in the new YouTube seek handler / dynamic right-panel mounting changes; those look consistent with the existing millisecond seek contract and YouTube sidebar remount behavior.
The prior P1 review thread on background.ts is still current/unresolved and still blocks LGTM: searches can run during the resumed import and cache negative dictionary misses until the MV3 worker restarts. Once that cache invalidation/gating issue is fixed, I’m happy to do the final pass.
Validation on f5e9959:
git diff --check github/main...HEADnpm run typecheckenv -u WATCH_REPORT_DEPENDENCIES npm -w @inkah/extension exec vitest run(99/99)env -u WATCH_REPORT_DEPENDENCIES npm run buildenv -u WATCH_REPORT_DEPENDENCIES NODE_ENV=production npm run build:web
…ner discovery Adds 6 happy-dom behavior tests driving VideoController through its public surface only (start, window events, settings toggle clicks): overlay mount + tokenized word rendering with background, native-subs fail-safe (no inkahsubs-active without cues), container-rename fallback, dynamic double-subtitle toggle, SPA right-panel remount, stop cleanup. These pin the render pipeline's input/output so refactors can prove behavior is unchanged. Netflix container discovery now falls back through .watch-video → .VideoContainer → [data-uia="video-canvas"] → video element ancestor, and every silent early-return in mountAll now logs a console warning so a broken pipeline is diagnosable from a single console capture. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
conoremclaughlin
left a comment
conoremclaughlin
left a comment
There was a problem hiding this comment.
Re-reviewed 97e3009.
No additional findings in the new VideoController behavior tests or Netflix container-discovery/logging hardening. The new tests are a useful guardrail around the render/fail-safe/right-panel behavior.
The prior P1 review thread on background.ts is still current/unresolved and still blocks LGTM: searches can run during the resumed dictionary import and cache negative dictionary misses until the MV3 worker restarts.
Validation on 97e3009:
git diff --check github/main...HEADnpm run typechecknpm ci(needed locally for the newhappy-domdev dependency)env -u WATCH_REPORT_DEPENDENCIES npm -w @inkah/extension exec vitest run(105/105)env -u WATCH_REPORT_DEPENDENCIES npm run buildenv -u WATCH_REPORT_DEPENDENCIES NODE_ENV=production npm run build:web
…btitles
Netflix renamed manifest fields (timedtexttracks→textTracks,
ttDownloadables→downloadables, urls object map→[{url}] array), which
silently killed all subtitle acquisition — hover, right panel, double
subs and backgrounds all starve without cues. Verified live against
netflix.com: tracks cache, 302 cues fetch, overlay renders.
- JSON.parse hook detects both manifest generations
- JSON.stringify hook finds the profiles array by content (ported from
subadub) instead of hardcoding params.profiles — Netflix renames
request fields too
- processSubData normalizes both track shapes and both urls variants
- Auto-pause now only triggers on target-language subtitle lines; the
secondary native-language line is read, not studied
- New tests pin BOTH manifest shapes so the next rename fails loudly
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
conoremclaughlin
left a comment
conoremclaughlin
left a comment
There was a problem hiding this comment.
Release re-review at 9cd75c4c796d3f27f18bccf21666e2f0c371728c.
I rechecked the latest Netflix manifest fix, YouTube seek/right-panel changes, and video-controller behavior hardening. I did not find any additional release blockers in the new 9cd75c4 delta, and local validation is green:
git diff --check github/main...HEADgit merge-tree --write-tree github/main HEADnpm run typecheckenv -u WATCH_REPORT_DEPENDENCIES npm -w @inkah/extension exec vitest run— 108/108 passedenv -u WATCH_REPORT_DEPENDENCIES npm run buildenv -u WATCH_REPORT_DEPENDENCIES NODE_ENV=production npm run build:web
I’m still not marking this LGTM for release because the prior P1 thread on packages/extension/src/entrypoints/background.ts:33 remains current and unresolved: startup import resume can still race with immediately-registered lookups, and dict-cache.ts can retain null misses created while dictionaries are still being rebuilt. Please clear the dictionary read-through caches after a successful resumed import, or gate/avoid negative caching until dictImportProgress.version >= IMPORT_VERSION.
…default size - YouTube: the injector auto-disabled native captions on load, and its own off-detection then dispatched an empty language change that wiped freshly-loaded cues — leaving the right panel (and sometimes the overlay) empty until a refresh. Native captions now stay enabled and the overlay CSS hides them, eliminating the race. Verified live: panel renders rows, seek carets jump playback. - Definition popup: final clamp against viewport edges using measured size, and mount inside document.fullscreenElement when fullscreen is active (nodes outside it don't render). - Default subtitle size 125% → 110%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
conoremclaughlin
left a comment
conoremclaughlin
left a comment
There was a problem hiding this comment.
Final re-review at c3afc20d6f1063627a4ca2c60e30c18f00891c09.
I rechecked the new final delta (content.ts, video-injector.content.ts, video-controller.ts) covering the YouTube cue-wipe fix, fullscreen/viewport popup placement, and the 110% default subtitle size. I did not find any additional release blockers in those changes. Local validation is green:
git diff --check github/main...HEADgit merge-tree --write-tree github/main HEADnpm run typecheckenv -u WATCH_REPORT_DEPENDENCIES npm -w @inkah/extension exec vitest run— 108/108 passedenv -u WATCH_REPORT_DEPENDENCIES npm run buildenv -u WATCH_REPORT_DEPENDENCIES NODE_ENV=production npm run build:web
However, I still can’t mark this release LGTM because the existing P1 thread on packages/extension/src/entrypoints/background.ts:33 is still current/unresolved and the final commit did not touch the import/cache path. The startup import resume still runs asynchronously while lookups are registered immediately, so dict-cache.ts can retain null misses produced during a resumed import. Please clear the dictionary read-through caches after a successful import, or gate/avoid negative caching until dictImportProgress.version >= IMPORT_VERSION.
…rgin The fullscreen YouTube rule stretched the settings logo to 28x54 — now 40x40 square (verified live in fullscreen). Right panel gets 16px bottom margin so it doesn't sit flush against the recommendations list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…v2.0.0 - Fullscreen YouTube logo renders at the same 32px as windowed mode (40px read as oversized/stretched); hit area stays 54px - YouTube settings dropdown anchors to the player's right edge instead of offsetting left from the icon — its right side no longer extends past the window - Version 2.0.0 for the MV3 architecture release Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mirror YouTube's own .ytp-button pattern (height: 100% of the controls row, vertical-align: top, flex-centered content) instead of fixed heights with baseline alignment — the icon now centers exactly in both windowed and fullscreen modes. Verified delta: 0px against the native fullscreen button midline. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…gative entries Resolves Lumen's P1: lookups served while the resumed dictionary import runs cache their misses as null in dict-cache.ts, and those negative entries shadowed freshly imported data until the MV3 worker restarted. doImport() now clears all read-through caches after completing; the background's existing warm pass then repopulates from the real tables. Adds a regression test: poisoned cache entry → import completes → lookup sees the imported row. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
conoremclaughlin
left a comment
conoremclaughlin
left a comment
There was a problem hiding this comment.
Final release pass at 1e937540852fafbf0827c056fa41a3182e1395d6: LGTM.
The prior P1 is fixed. clearDictionaryCaches() now clears all dictionary read-through maps after a successful import/version stamp, so any null misses cached during a resumed import are dropped before the warmed cache is repopulated from the real tables. The new regression test covers the poisoned-negative-cache case: a lookup misses while import progress is incomplete, the row lands in IndexedDB, the import completes, and the next lookup returns the imported row. I resolved the previous review thread.
I also rechecked the post-c3afc20 release polish (YouTube settings icon centering/fullscreen sizing, dropdown anchoring, right-panel bottom margin, and extension version bump to 2.0.0) and found no new blockers.
Validation passed locally:
git diff --check github/main...HEADgit merge-tree --write-tree github/main HEADnpm run typecheckenv -u WATCH_REPORT_DEPENDENCIES npm -w @inkah/extension exec vitest run— 109/109 passedenv -u WATCH_REPORT_DEPENDENCIES npm run buildenv -u WATCH_REPORT_DEPENDENCIES NODE_ENV=production npm run build:web
Ready for merge/store-submission from my side.
1 participant
Summary
Follow-ups from Lumen's review of #2, plus two reliability fixes ahead of extension store submission:
Cleanup
effector/effector-react— declared but never importedpackages/web/out— generated Next.js output shouldn't be committed (Lumen's trailing-whitespace flag)Reliability fixes
inkahsubs-activeclass set only while Inkah is actively rendering cues. If the subtitle pipeline breaks (e.g. a Netflix player change), users keep native subtitles instead of getting nothing.onInstalled. MV3 kills workers mid-import (Vicon is 61MB / 707K entries) and nothing ever retried, leaving Korean lookups silently empty. In-flight guard dedupes the onInstalled/startup race.Test plan
npm run typecheckpassesnpm test— 99/99 pass (3 new import-resume regression tests)npm run buildsucceeds🤖 Generated with Claude Code